daemon, o/snapstate: implement snapstate.InstallPath in terms of refresh by andrewphelpsj · Pull Request #17166 · canonical/snapd

andrewphelpsj · 2026-06-05T14:49:28Z

This lets us use the existing seed-refresh implementation for free during single-path installation. Tests are added for this behavior.

codecov · 2026-06-05T15:10:39Z

Codecov Report

❌ Patch coverage is 97.87234% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.11%. Comparing base (6982854) to head (42c7e32).
⚠️ Report is 21 commits behind head on master.

Files with missing lines	Patch %	Lines
overlord/snapstate/target.go	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #17166   +/-   ##
=======================================
  Coverage   79.11%   79.11%           
=======================================
  Files        1389     1391    +2     
  Lines      193979   193983    +4     
  Branches     2466     2466           
=======================================
+ Hits       153461   153465    +4     
  Misses      31312    31312           
  Partials     9206     9206

Flag	Coverage Δ
unittests	`79.11% <97.87%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

miguelpires

question

github-actions · 2026-06-05T17:11:31Z

Mon Jun 29 15:41:10 UTC 2026
The following results are from: https://github.com/canonical/snapd/actions/runs/28115261629

Failures:

Preparing:

openstack-arm-ext:ubuntu-24.04-arm-64:tests/nested/manual/optee-fde:optee
openstack-arm-ext:ubuntu-24.04-arm-64:tests/nested/core/core20-basic
openstack-arm-ext:ubuntu-24.04-arm-64:tests/nested/manual/optee-fde:hooks
openstack:opensuse-tumbleweed-64:
openstack:opensuse-tumbleweed-64:
openstack:opensuse-tumbleweed-64:
openstack:opensuse-tumbleweed-selinux-64:
openstack:opensuse-tumbleweed-selinux-64:
openstack:opensuse-tumbleweed-selinux-64:
openstack:opensuse-tumbleweed-64:
openstack:opensuse-tumbleweed-selinux-64:
openstack:opensuse-tumbleweed-selinux-64:
openstack:opensuse-tumbleweed-64:
openstack:opensuse-tumbleweed-selinux-64:
openstack:opensuse-tumbleweed-64:

Restoring:

openstack:opensuse-tumbleweed-64:
openstack:opensuse-tumbleweed-64:
openstack:opensuse-tumbleweed-64:
openstack:opensuse-tumbleweed-64:
openstack:opensuse-tumbleweed-selinux-64:
openstack:opensuse-tumbleweed-selinux-64:
openstack:opensuse-tumbleweed-selinux-64:
openstack:opensuse-tumbleweed-selinux-64:
openstack:opensuse-tumbleweed-64:

Skipped tests from snapd-testing-skip

If you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list of the below tests you wish to run (unskip plus test list must be valid yaml)

garden:ubuntu-25.10-64:tests/main/apparmor-prompting-support
openstack-arm:ubuntu-24.04-arm-64:tests/main/i18n
openstack-arm:ubuntu-core-24-arm-64:tests/main/i18n
openstack:debian-sid-64:tests/main/interfaces-network-status-classic
openstack:debian-sid-64:tests/main/interfaces-xdg-portal-permission-store
openstack:ubuntu-24.04-64:tests/main/i18n
openstack:ubuntu-25.10-64:tests/main/apparmor-prompting-flag-restart
openstack:ubuntu-25.10-64:tests/main/apparmor-prompting-integration-tests
openstack:ubuntu-25.10-64:tests/main/apparmor-prompting-prompt-restoration
openstack:ubuntu-25.10-64:tests/main/apparmor-prompting-smoke
openstack:ubuntu-25.10-64:tests/main/apparmor-prompting-snapd-startup
openstack:ubuntu-25.10-64:tests/main/apparmor-prompting-support
openstack:ubuntu-25.10-64:tests/main/interfaces-requests-activates-handlers
openstack:ubuntu-26.04-64:tests/main/apparmor-prompting-flag-restart
openstack:ubuntu-26.04-64:tests/main/apparmor-prompting-integration-tests
openstack:ubuntu-26.04-64:tests/main/apparmor-prompting-prompt-restoration
openstack:ubuntu-26.04-64:tests/main/apparmor-prompting-smoke
openstack:ubuntu-26.04-64:tests/main/apparmor-prompting-snapd-startup
openstack:ubuntu-26.04-64:tests/main/apparmor-prompting-support
openstack:ubuntu-26.04-64:tests/main/i18n
openstack:ubuntu-26.04-64:tests/main/interfaces-requests-activates-handlers

miguelpires

thanks

pedronis

behavior and symmetry question

pedronis · 2026-06-11T15:24:15Z

 // Note that the state must be locked by the caller.
 // The provided SideInfo can contain just a name which results in a
 // local revision and sideloading, or full metadata in which case it
 // the snap will appear as installed from the store.


the comment here probably needs to explain the behavioral differences between this and InstallOne with PathInstallGoal or maybe the issue is that we want InstallOne with that to also do the right thing?

I'll update the comment on the differences, though I'm not clear on what we want InstallOne with that to also do the right thing might mean?

Do you mean that we should consider making that trigger a seed-refresh?

will need to be reviewed from scratch

andrewphelpsj · 2026-06-15T22:57:38Z

This PR is kinda a mixture of things now, not super focused. Though if the goal is to keep snapstate's functions more consistent, I do think they should probably be grouped together. Commits will read better than the full thing.

andrewphelpsj · 2026-06-15T23:03:46Z

To explain the refactor a bit:

I've reimplemented the current remodeling functionality that uses the snapstate's install goals in terms of update goals.

Then, I've renamed some things in snapstate to make the asymmetry more reasonable. The new snapstate.SeedingGoal is introduced to replace the current snapstate.PathInstallGoal, and then I've renamed the existing snapstate.Install* functions that no longer actually go through just installation.

miguelpires

looks fine, just one question

andrewphelpsj · 2026-06-16T15:31:57Z

Reordering some things, #17210 will get merged first.

pedronis

did a pass, couple of comments/questions, not sure the last rename is a pure win

pedronis · 2026-06-18T16:22:14Z

+	// snap state, so set that fallback before resolving.
 	if update.RevOpts.Channel == "" {
-		update.RevOpts.Channel = update.SideInfo.Channel
+		update.RevOpts.Channel = firstNonEmpty(update.SideInfo.Channel, trackingChannel)


should not this be done as some flag to resolveChannel perhaps? the comment is not super clear to me

I'm not sure I can come up with a good name that isn't super specific to this use-case.

The issue is that we use this RevisionOptions.resolveChannel method from here and from within the store-based goals. In the store based goals, we skip the initial channel resolution during by-revision actions, since we can't know what channel to send to the store. We have a secondary resolution phase after we've finished talking to the store:

snapd/overlord/snapstate/target.go

Lines 202 to 219 in 60ca9aa

trackedChannel := revOpts.Channel

if action == "refresh" {

// if we still have no channel here, this means that we refreshed

// by-revision without specifying a channel. make sure we continue to

// track the channel that the snap is currently on

trackedChannel = firstNonEmpty(

trackedChannel,

snapst.TrackingChannel,

)

} else {

trackedChannel = firstNonEmpty(

sar.RedirectChannel,

trackedChannel,

// fallback to "stable" should only happen if the caller requested a

// specific revision to be installed, without specifying a channel.

"stable",

)

}

We should not skip resolution here, since we want to either take what is given to us, or use the already tracked channel. There isn't a secondary resolution phase for by-path installs, so we have to make the decision here.

Something like forceChannelResolution bool could work? Let me know and I'll make that change, but whatever we do here will be a bit confusing, in my opinion.

the problem I have is that this new logic is called for all the cases targetForPathSnap is called but afaik there's a clear need only for seedingGoal, it's hard to reason if this is correct for the other case or not, or am I reading the changes wrong?

Talked in person, will rename some methods on RevisionOptions and then add explicit test for PathUpdateGoal that sets a RevisionOpts.Revision.

Both are done now.

pedronis · 2026-06-18T16:28:13Z

 }

-// InstallPath returns a set of tasks for installing a snap from a file path.
+// UpdatePath returns a set of tasks for updating a snap from a file path.


I'm not sure the rename is that useful, I think maybe we should just be clearer in the doc comment of the function that this is able also to update snap if it's already installed

I dropped the commit with the rename and updated the comment.

This lets us use the existing seed-refresh implementation for free during single-path installation. Tests are added for this behavior.

This makes the intent clearer, since it is only used during seeding now. Options.Seed is pretty lame though, now.

pedronis

question again

pedronis · 2026-06-24T14:32:20Z

+	// snap state, so set that fallback before resolving.
 	if update.RevOpts.Channel == "" {
-		update.RevOpts.Channel = update.SideInfo.Channel
+		update.RevOpts.Channel = firstNonEmpty(update.SideInfo.Channel, trackingChannel)


the problem I have is that this new logic is called for all the cases targetForPathSnap is called but afaik there's a clear need only for seedingGoal, it's hard to reason if this is correct for the other case or not, or am I reading the changes wrong?

…of refresh

…ed revision in PathUpdateGoal

pedronis

question about the new test

pedronis · 2026-06-24T16:21:29Z

+	c.Assert(err, IsNil)
+	c.Check(snapsup.InstanceName(), Equals, snapName)
+	c.Check(snapsup.Revision(), Equals, rev)
+	c.Check(snapsup.SnapPath, Equals, snapPath)


do we need to assert something about channels as that was the relevant code that triggered adding this?

I added an assertion that ensures we retain the tracked channel in this case.

…installed revision in PathUpdateGoal

pedronis

thank you

github-actions Bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jun 5, 2026

andrewphelpsj requested review from miguelpires and pedronis June 5, 2026 15:04

miguelpires reviewed Jun 5, 2026

View reviewed changes

Comment thread overlord/snapstate/seed.go Outdated

Comment thread overlord/snapstate/snapstate.go

andrewphelpsj requested a review from miguelpires June 5, 2026 17:06

andrewphelpsj force-pushed the seed-refresh-file-update branch 2 times, most recently from 0894708 to 5b7a043 Compare June 10, 2026 16:41

miguelpires previously approved these changes Jun 11, 2026

View reviewed changes

pedronis reviewed Jun 11, 2026

View reviewed changes

andrewphelpsj force-pushed the seed-refresh-file-update branch from 5b7a043 to cf03f3b Compare June 11, 2026 16:41

andrewphelpsj requested a review from pedronis June 11, 2026 16:41

pedronis removed their request for review June 15, 2026 11:58

andrewphelpsj force-pushed the seed-refresh-file-update branch 2 times, most recently from 10e73cb to 02ffcb0 Compare June 15, 2026 22:57

andrewphelpsj requested review from miguelpires and pedronis June 15, 2026 22:57

andrewphelpsj force-pushed the seed-refresh-file-update branch from 02ffcb0 to 83cac3c Compare June 15, 2026 23:03

miguelpires reviewed Jun 16, 2026

View reviewed changes

Comment thread overlord/snapstate/target.go

andrewphelpsj force-pushed the seed-refresh-file-update branch 4 times, most recently from ab3f135 to ee62b2c Compare June 18, 2026 16:14

pedronis reviewed Jun 18, 2026

View reviewed changes

andrewphelpsj force-pushed the seed-refresh-file-update branch from ee62b2c to e7b8f9e Compare June 18, 2026 18:51

andrewphelpsj added 5 commits June 24, 2026 10:01

daemon, o/snapstate: implement snapstate.InstallPath in terms of refresh

affe379

This lets us use the existing seed-refresh implementation for free during single-path installation. Tests are added for this behavior.

o/snapstate: add Options.NoSeedRefresh to fully disable seed refresh

e1b2fe2

o/snapstate: drop unused InstallWithDeviceContext function

28a40bf

o/snapstate, o/devicestate: repurpose PathInstallGoal as SeedingGoal

ba443c7

This makes the intent clearer, since it is only used during seeding now. Options.Seed is pretty lame though, now.

o/snapstate: note that InstallPath functions can be used to update

08849e0

andrewphelpsj force-pushed the seed-refresh-file-update branch from e7b8f9e to 08849e0 Compare June 24, 2026 14:01

andrewphelpsj requested review from miguelpires and pedronis June 24, 2026 14:02

pedronis reviewed Jun 24, 2026

View reviewed changes

andrewphelpsj added 2 commits June 24, 2026 12:03

fixup! daemon, o/snapstate: implement snapstate.InstallPath in terms …

ddc53bd

…of refresh

o/snapstate: test setting RevisionOptions.Revision to already install…

ae2d798

…ed revision in PathUpdateGoal

andrewphelpsj requested a review from pedronis June 24, 2026 16:10

pedronis reviewed Jun 24, 2026

View reviewed changes

fixup! o/snapstate: test setting RevisionOptions.Revision to already …

42c7e32

…installed revision in PathUpdateGoal

andrewphelpsj requested a review from pedronis June 24, 2026 16:56

pedronis approved these changes Jun 25, 2026

View reviewed changes

miguelpires approved these changes Jun 26, 2026

View reviewed changes

github-actions Bot added the Auto rerun spread Auto reruns spread up to 4 times in non-draft PRs w/ >=1 approval and <20 fails in any fund. system label Jun 26, 2026

andrewphelpsj merged commit 4c97667 into canonical:master Jun 29, 2026
616 of 673 checks passed

andrewphelpsj deleted the seed-refresh-file-update branch June 29, 2026 15:49

	trackedChannel := revOpts.Channel
	if action == "refresh" {
	// if we still have no channel here, this means that we refreshed
	// by-revision without specifying a channel. make sure we continue to
	// track the channel that the snap is currently on
	trackedChannel = firstNonEmpty(
	trackedChannel,
	snapst.TrackingChannel,
	)
	} else {
	trackedChannel = firstNonEmpty(
	sar.RedirectChannel,
	trackedChannel,
	// fallback to "stable" should only happen if the caller requested a
	// specific revision to be installed, without specifying a channel.
	"stable",
	)
	}

Uh oh!

Conversation

andrewphelpsj commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

miguelpires left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Failures:

Preparing:

Restoring:

Skipped tests from snapd-testing-skip

Uh oh!

miguelpires left a comment

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewphelpsj commented Jun 15, 2026

Uh oh!

andrewphelpsj commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miguelpires left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewphelpsj commented Jun 16, 2026

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

andrewphelpsj commented Jun 5, 2026 •

edited

Loading

codecov Bot commented Jun 5, 2026 •

edited

Loading

github-actions Bot commented Jun 5, 2026 •

edited

Loading

andrewphelpsj commented Jun 15, 2026 •

edited

Loading